Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes up InverseDynamicsController to use the actuation matrix #22315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Dec 15, 2024

Clarifies that the control output of the inverse dynamics controller
is actuation, not generalized forces, and uses the actuation matrix to
achieve this. Previously, by connecting the output of the
InverseDynamicsController system to an actuation input port of the
MultibodyPlant, we implicitly assumed that the actuator matrix was the
identity matrix. If the actuators were in a different order than the
joints (generalized forces), then our method produced the wrong
actuator inputs.

The generalized_force output port is still available, partly for
backwards compatibility, and because it's possible that there are
applications that prefer this.

This is a rewrite of PR #20971.
+@sherm1 for feature review, please (as the reviewer of #20971)
cc @EricCousineau-TRI who was involved in the first PR.
cc @siddancha

Note: I will follow-up with changes to Anzu as needed.


This change is Reviewable

Clarifies that the control output of the inverse dynamics controller
is actuation, not generalized forces, and uses the actuation matrix to
achieve this. Previously, by connecting the output of the
InverseDynamicsController system to an actuation input port of the
MultibodyPlant, we implicitly assumed that the actuator matrix was the
identity matrix. If the actuators were in a different order than the
joints (generalized forces), then our method produced the wrong
actuator inputs.

The generalized_force output port is still available, partly for
backwards compatibility, and because it's possible that there are
applications that prefer this.

This is a rewrite of PR RobotLocomotion#20971.
@RussTedrake
Copy link
Contributor Author

+@sherm1 for feature review, please?

Copy link
Contributor

@siddancha siddancha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an official reviewer. But caught a couple of things (that might not turn out to be real issues).

Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)


systems/controllers/inverse_dynamics_controller.h line 35 at r1 (raw file):

 * inverse dynamics computation.
 *
 * @note In the case where the plant is not fully actuated, then B⁻¹ is

Docs currently states that "this class assumes the robot is fully actuated" [1, 2]. Is that not true anymore since we're now using an actuation matrix that need not be full rank? Or we do we still need to assume the plant is fully actuated, in which case this comment is redundant?


systems/controllers/test/inverse_dynamics_controller_test.cc line 76 at r1 (raw file):

        robot_context == nullptr ? robot_plant.CreateDefaultContext()
                                 : robot_context->Clone();
    VectorX<double> expected_force = controllers_test::ComputeTorque(

Is it weird that we have "force" on the left hand side, and "ComputeTorque" on the right hand side?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants